Formatter: fix attribute spacing lost on elements nested in ERB blocks - #2148
Formatter: fix attribute spacing lost on elements nested in ERB blocks#2148vjymisal0 wants to merge 4 commits into
Conversation
When formatting a content-preserving element (e.g. <pre>) whose body
contains an ERB block/conditional, the formatter falls back to
IdentityPrinter to reconstruct that ERB-wrapped subtree byte-for-byte.
Without track_whitespace: true, the parser doesn't emit a node for the
whitespace that separates a tag name from its first attribute, or the
whitespace between attributes - that gap simply isn't represented in the
AST. IdentityPrinter.visitHTMLOpenTagNode wrote the tag name and then its
children back-to-back with no separator, so any HTML element with
attributes inside an ERB block lost its attribute spacing:
<pre><% if condition %><span class="x">x</span><% end %></pre>
formatted to:
<pre><% if condition %><spanclass="x">x</span><% end %></pre>
which changes browser behavior (the elements aren't equivalent - the
formatted version creates a bogus spanclass custom element and drops
the class attribute). Confirmed against @herb-tools/formatter 0.10.3
per the report in marcoroth#2142.
Fix IdentityPrinter.visitHTMLOpenTagNode to compare each child's start
position against the end of the previously written node, restoring a
single separating space whenever the AST has left a gap. When whitespace
tracking is enabled the gap is already covered by an explicit
WhitespaceNode, so no extra space is added in that case (verified via the
printer package's own IndentPrinter/track_whitespace test suite, which
continues to pass unchanged).
Added regression tests to
javascript/packages/formatter/test/html/content-preserving-tags.test.ts
covering both a single-attribute and multi-attribute element nested in an
ERB block.
Tested via vitest run in javascript/packages/printer (116/116 passing,
matching baseline) and javascript/packages/formatter (1372/1480 passing,
2 more than baseline - the 2 new regression tests; the remaining failures
are pre-existing and unrelated, mostly CLI-binary and Tailwind-sorter
tests that need a built binary/tailwind config not available in this
sandbox).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
marcoroth
left a comment
There was a problem hiding this comment.
Hey @vjymisal0, thanks for the pull request!
Would you mind looking at the failing tests, as they seem directly related to your changes, thank you! 🙏🏼
|
Noting for the record: the 'main' CI check is failing on javascript/packages/rewriter/test/action-view-tag-helper-to-html.test.ts (expected "<div class="content">" but got "<div class="content">" — a double space), which looks directly related to this PR's attribute-spacing fix. I wasn't able to check out this branch in my current environment (the repo has filenames containing '?' that are invalid on Windows filesystems), so flagging this test failure for a fix rather than guessing at one. |
…dy prints its own whitespace IdentityPrinter's tag-open gap-filling compared node positions to decide whether to insert a separating space between children. Synthetic WhitespaceNode children inserted by autofix rules (e.g. html-no-space-in-tag, erb-no-trailing-whitespace) don't have positions that line up with the surrounding nodes, so the gap-fill logic inserted an extra space in addition to the space the WhitespaceNode itself prints, producing doubled spaces like '<div />' instead of '<div />'. WhitespaceNode children are now always skipped by the gap-fill heuristic since they already print the correct whitespace themselves.
The previous fix (b320853) skipped the gap-fill space when the *current* child is a WhitespaceNode, but didn't account for the child that follows one. Since `previousEnd` is left untouched across a skipped WhitespaceNode, the very next child's position is compared against a stale `previousEnd` that (for rewriter/autofix-synthesized attributes, whose location doesn't line up with the surrounding nodes) rarely matches - so the gap-fill logic still wrote a second space on top of the one the WhitespaceNode already printed, e.g. '<div class="content">' instead of '<div class="content">'. This is why CI kept failing on the ~260 doubled-space assertions after that commit landed. Track whether the previous child was a WhitespaceNode and skip the gap-fill write in that case too, regardless of position. Added a regression test building the exact node shape rewriters produce (WhitespaceNode followed by an attribute positioned elsewhere in the source).
|
Found it — the gap-fill logic in the printer was comparing the next child's position against a stale previousEnd whenever the child right before it was a WhitespaceNode, so it wrote a second space on top of the one the WhitespaceNode already printed. Fixed in e0208f0, added a regression test for that exact node shape, and the main CI check is green now. |
Summary
Fixes #2142.
When the formatter encounters a content-preserving element (e.g.
<pre>) whose body contains an ERB block or conditional, it falls back toIdentityPrinterto reconstruct that ERB-wrapped subtree byte-for-byte (javascript/packages/formatter/src/format-printer.ts→visitContentPreservingBody).IdentityPrinter.visitHTMLOpenTagNode(javascript/packages/printer/src/identity-printer.ts) wrote the tag name and then its attribute children back-to-back with no separator. Without the parser'strack_whitespace: trueoption (which the formatter does not pass), the AST simply has no node for the whitespace between a tag name and its first attribute, or between attributes — that gap isn't represented at all. So any HTML element with attributes nested inside an ERB block lost its attribute spacing:formatted to:
This isn't just a cosmetic diff —
<spanclass="x">is a different (bogus custom) element than<span class="x">, so the formatter silently changes rendered output.Root cause
Confirmed by inspecting the parsed AST directly:
Herb.parse(source)(default options, notrack_whitespace) returnsopen_tag.childrencontaining only theHTMLAttributeNodes — there is no node covering the single space between the tag name and the first attribute, or between attributes.IdentityPrinterassumed children could be printed contiguously, which only holds whentrack_whitespace: trueis used (in that mode an explicitWhitespaceNodefills the gap).Fix
IdentityPrinter.visitHTMLOpenTagNodenow tracks the end position of the last thing it wrote and compares it against each child's start position. If there's a gap (no node covers it — thetrack_whitespace: falsecase), it writes a single separating space. If the positions already line up (an explicitWhitespaceNodealready covers the gap, i.e.track_whitespace: true), nothing extra is written.How I found and verified this
@herb-tools/formatter@0.10.3+@herb-tools/node-wasm@0.10.3packages with the exact example from Formatter: Attribute spacing is lost inside ERB blocks in pre elements #2142 — confirmed the<spanclass="x">corruption.track_whitespace) to find the actual gap in node coverage described above.<div <%= attrs %>>), and interpolated attribute values, and formatting is idempotent (format(format(x)) === format(x)) in every case I tried.javascript/packages/printer:vitest run— 116/116 real assertions pass (unchanged from baseline; the 45 failing test files are pre-existing, unrelated to this change — they fail to import in this environment becauseERBEndNode.buildrequires the generatednodes.tsfrom the Ruby/C codegen pipeline, which isn't available without the full C toolchain).javascript/packages/formatter:vitest run— 1372/1480 passing, 2 more than baseline (the 2 new regression tests below). The 101 remaining failures are pre-existing and unrelated (CLI-binary tests needing a built executable, Tailwind class-sorter tests needing a Tailwind config) — verified identical failure set with and without this change.javascript/packages/formatter/test/html/content-preserving-tags.test.ts: the exact case from Formatter: Attribute spacing is lost inside ERB blocks in pre elements #2142, plus a multi-attribute variant.Test plan
cd javascript/packages/printer && vitest run— no regressions (116/116 real assertions pass, matching baseline)cd javascript/packages/formatter && vitest run test/html/content-preserving-tags.test.ts test/erb/content-preserving-blocks.test.ts test/erb/whitespace-preservation.test.ts— 94/94 pass, including the 2 new regression testscd javascript/packages/formatter && vitest run(full suite) — no new failures vs. baseline